-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Example of notifying with terminal-notifier. #104
base: master
Are you sure you want to change the base?
Conversation
Hi @stoyle, Thanks for working on this. I'm not yet sure if it is something we should add to lein-doo. I will test it (both from the PR and in |
I don't think lein-autotest does this (I may be wrong though), but I do know lein-test-refresh does. I do understand doo is complex as it is, and maybe you don't want to add to the complexity. But still I hope this will make it in, in one form or the other. My colleagues and I are often "annoyed" that we don't get notifications when our tests break, since we don't always watch the terminal. If you do decide to add this feature, and you want help adding it, I will be happy to help you implement it in any way you'd like. If you're uncomfortable adding it directly, one suggestions would be to a plugin of sorts, but it does need to have a hook into here. Cheers, |
Added a few more commits. It now "works on my machine" :) Guess I'll have to wait for the CircleCI build to see that I didn't break anything. Struggled a bit passing the command line args, but this works, preserving the previous arg parsing. Crossing my fingers you will accept this, or give me pointers on how to make it acceptable. If not, we probably will build our own, and deploy to our internal artifact server. Cheers, |
1f93e62
to
895bb02
Compare
Having tried this branch out locally, I have to say it is quite nice. Have been missing this since we switched over to pure cljs-tests. |
Hi @stoyle I found the time to try this on OSX and it works well. Here are some changes to pull out the notifications to the outermost layer of the plugin. I'm not sure how to push them to this PR. This PR prompts us to making three more changes:
I just pushed a new version for |
Hi @bensu. I understand your concern. Some friends and me have been discussing to create a separate library for this. One which could be configured through leiningen, e.g. What we would need is a "stable" insertion point into the code to extend the functionality, e.g. a multimethod. It could also be done through alter-var-root, or something like that, but that's kind of dirty. Anyways, I guess it is up to you. I am off to holidays now, so probably won't have time to look at it for a few weeks. Let me know what you decide. Cheers, |
@stoyle I thought about that as well and that is in part why I decided to move |
Reading notify and change-only from passed in opts. Also not comparing new with new, getting the old value from the atom before setting it.
Passing it through to the library through the opts map, in order for the notifier functionality to be able to pick it up.
Hi @bensu, ready for some work again ;) Updated the PR with your change, and rebased it off the current master. Guess you should add a 1.8.0-SNAPSHOT before merging anything? Did a quick test, and everything seems to be working fine. Before summer I was talking with Anders, who made flare. It basically uses the technique we were thinking about. You can configure it in your .lein/profiles.clj and it will always be on. If we'd want it to not be so static, we could allow overriding it by usind Anders was talking about doing this, inspired by my code, since he had some time on his hand. Not sure if he has though... Will ask him if he's had any progress when summer is over. By the way he is working on making flare work with cljs.test as well, which will be great. It is a lib which minimizes the diffs for all clojure datastructures. Anyways, how would you like to go forward? The code in this PR does work as expected. If you want it in, go ahead and merge it. If not, we will probably make an external lib for it. In the latter case it would be nice if you gave a stable "external" extension point we could extend doo with. |
Any chance this could get resurrected? Notifications for test runners running in the background is really handy |
I see there are a few conflicts here. If I fix them, any chance it will be merged? Cheers, |
One thing I really miss in doo from lein-autoexpect is growl/terminal notifications. So I had a look at doo, and it seems not too hard to support this also in doo.
The idea is this is optional params to the doo command. E.g.
lein doo slimer test notify change-only
, where says to use the terminal-notifier (could also be growl), and change-only will only notify when the test status actually changes.This, of course, is not ready to be merged. But before I do any more work, I would like to know if this feature is welcomed.
If so, I can prepare a complete working PR, and any guidance is of course welcome.
Cheers,
Alf